Fix Agent close hanging#934
Conversation
|
@boks1971 I'll get this improved ready for review + regressions tests by tomorrow. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #934 +/- ##
==========================================
- Coverage 88.33% 88.10% -0.23%
==========================================
Files 46 46
Lines 5973 6071 +98
==========================================
+ Hits 5276 5349 +73
- Misses 482 496 +14
- Partials 215 226 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
82125a2 to
5168f71
Compare
5168f71 to
57a11b7
Compare
|
@boks1971 the new tests in agent_test deadblock on main. which confirms the fix i think also with the results from your dev env. |
There was a problem hiding this comment.
Pull request overview
This PR addresses an Agent shutdown deadlock by aborting started candidate socket I/O before waiting for the task loop’s active task to return, preventing Close() from hanging when a task is blocked in WriteTo.
Changes:
- Add
CloseWithPreStoptotaskloop.Loopand use it fromAgent.close()to run a pre-stop hook before waiting for the loop to stop. - Track started candidates in
Agentand abort their I/O during shutdown to unblock in-flight socket operations. - Add abort-write plumbing for muxed/shared packet conns and introduce regression tests for blocked candidate writes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| udp_muxed_conn.go | Adds abortWrite() forwarding to the UDP mux to help unblock muxed writes during shutdown. |
| udp_mux.go | Implements abortWrite() via write deadlines to interrupt blocked WriteTo on the shared UDP socket. |
| shared_packet_conn.go | Adds abortWrite() passthrough when the underlying conn supports write abortion. |
| internal/taskloop/taskloop.go | Introduces CloseWithPreStop and sync.Once to ensure close/preStop are executed once safely. |
| internal/taskloop/taskloop_test.go | Adds a concurrency test for CloseWithPreStop behavior (currently contains a compile issue). |
| candidate_base.go | Refactors candidate close into abortIO() + wait, and adds write-abort support via writeAborter. |
| agent.go | Tracks started candidates and aborts their I/O as a pre-stop hook during agent shutdown. |
| agent_test.go | Adds regression tests ensuring Agent close unblocks blocked writes (direct, shared, and UDP mux). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var wg sync.WaitGroup | ||
| wg.Add(closers) | ||
| for range closers { | ||
| go func() { | ||
| defer wg.Done() | ||
| loop.CloseWithPreStop(func() { | ||
| preStopCalls.Add(1) | ||
| }) | ||
| closeReturned <- struct{}{} | ||
| }() | ||
| } |
boks1971
left a comment
There was a problem hiding this comment.
lgtm!
But, the CoPilot flagged UDPMux seemed like something legit?
| close(releaseBlock) | ||
| wg.Wait() | ||
|
|
||
| assert.Equal(t, int32(1), preStopCalls.Load()) |
Yeah I'm adding an atomic counter. |
Abort started candidate i/o before waiting for the task loop to stop. this prevents close from deadblocking when a task is blocked in WriteTo
57a11b7 to
24d50e5
Compare
|
@boks1971 i added the atomic counter, can you please review / test this again, i'll manually test this a lot before i merge it. |
Thank you @JoTurk . The changes lgtm! The Gosched calls are always a bit scary. Guess the alternate is not very useful here. Would be good to get 👀 from @cnderrauber again as he has spent a bunch of time in these mux modules. My set up seems to be in a flux state currently. I will have to try and repro the failure I had before. But, please do not wait for me. I will report back issues when I get a chance to try. |
|
Is there a problem if we just left udpmux unchanged (or empty implementation of |
The original theory that forced these set of changes from Jo was that the STUN ping got blocked on a close of an ICE agent. Looks like golang can block UDP write, but has to be very specific conditions like kernel buffer exhaustion which I think should be very rare. But, that was the best theory for hangs in LiveKit server on participant close (especially participants who were not disconnecting cleanly). |
|
Sounds good. I have been concerned the loop hang on close when the write is blocked before, but thought the udp socket would not be blocked. The change looks good to me! |
Yes I tried to reproduce it with stressing writes, and I think it has to do with OS buffer, and go handling of EAGAIN, I think it's a bug at the Go runtime, there are a few reports that I found golang/go#73919 golang/go#61555 I'm not sure why it happens more on darwin but i was able to replicate it once on linux. |
Description
Abort started candidate i/o before waiting for the task loop to stop.
this prevents close from deadblocking when a task is blocked in WriteTo
This is a fix i have been testing for a few days it's missing regression test and some improvements.